Skip to content

Fix: fold simulated device logs into HostLogger - #2061

Merged
ChaoWao merged 2 commits into
hw-native-sys:mainfrom
indigo1973:0828
Aug 31, 2026
Merged

Fix: fold simulated device logs into HostLogger#2061
ChaoWao merged 2 commits into
hw-native-sys:mainfrom
indigo1973:0828

Conversation

@indigo1973

Copy link
Copy Markdown
Contributor
  • Bind each simulation AICPU DSO to the process-owned HostLogger state before applying the compatibility log level.
  • Route simulation device records through HostLogger so they share the host envelope, live threshold, and configured destination.
  • Remove the duplicate simulation formatter/writer and its 2048-byte PIPE_BUF constraint while leaving the onboard CANN backend unchanged.
  • Add live-threshold, envelope, concurrent-thread, and forked-process coverage for issue [Code Health] Log subsystem — one clock, one grammar, one gate #1792 item 5.
  • Fully initialize existing ABI v2 log-directory fields for strict simulation AICPU builds.

@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9e6be113-3642-44e1-85b7-a3ae3939cf44

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 09393f81-6715-4c4b-b71d-e3e86072b09a

📥 Commits

Reviewing files that changed from the base of the PR and between 89ba2a3 and 04a920a.

📒 Files selected for processing (11)
  • src/a2a3/platform/sim/aicpu/CMakeLists.txt
  • src/a2a3/platform/sim/host/device_runner.cpp
  • src/a5/platform/sim/aicpu/CMakeLists.txt
  • src/a5/platform/sim/host/device_runner.cpp
  • src/common/log/host_log.cpp
  • src/common/platform/include/aicpu/device_log.h
  • src/common/platform/onboard/aicpu/device_log.cpp
  • src/common/platform/shared/aicpu/unified_log_device.cpp
  • src/common/platform/sim/aicpu/device_log.cpp
  • tests/ut/cpp/CMakeLists.txt
  • tests/ut/cpp/common/test_sim_device_log.cpp

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Changes

The AICPU simulation logger now delegates severity checks, state binding, level updates, formatting, and output to HostLogger. Both simulation builds include host_log.cpp. The public logging interface now exposes severity query functions. Tests cover envelopes, thresholds, concurrency, fork handling, and pipe output.

HostLogger-backed AICPU logging

Layer / File(s) Summary
Logging contract and state initialization
src/common/platform/include/aicpu/device_log.h, src/common/platform/onboard/aicpu/device_log.cpp, src/common/log/host_log.cpp, src/common/platform/shared/aicpu/unified_log_device.cpp
Severity checks use platform-specific query functions. The shared host log state initializes its directory fields. Comments describe the HostLogger-backed simulation path.
Simulation HostLogger integration
src/common/platform/sim/aicpu/device_log.cpp, src/a2a3/platform/sim/host/device_runner.cpp, src/a5/platform/sim/host/device_runner.cpp, src/a2a3/platform/sim/aicpu/CMakeLists.txt, src/a5/platform/sim/aicpu/CMakeLists.txt
Simulation logging binds host state before applying the level. Log output and filtering use HostLogger. Both simulation libraries compile host_log.cpp.
Simulation logging validation
tests/ut/cpp/CMakeLists.txt, tests/ut/cpp/common/test_sim_device_log.cpp
Tests bind shared logging state and validate filtering, envelopes, live threshold changes, multithreaded output, fork behavior, and small-pipe handling.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: 🔵 Low · up to 04a92

Simulation and host records now target the same log destination, but separate buffered writers may produce out-of-order or delayed diagnostic records during concurrent, reload, or failure scenarios. The change is otherwise mergeable with explicit owner awareness and follow-up on shared-sink serialization and flush behavior.

Sequence Diagram(s)

sequenceDiagram
  participant device_runner
  participant AICPU_sim_device_log
  participant HostLogger
  device_runner->>AICPU_sim_device_log: set_host_log_state(state)
  AICPU_sim_device_log->>HostLogger: bind_state(state)
  device_runner->>AICPU_sim_device_log: set_log_level(level)
  AICPU_sim_device_log->>HostLogger: set_level(level)
  AICPU_sim_device_log->>HostLogger: vlog(level, message)
Loading

Poem

A rabbit binds the logger tight

Thresholds guide the stream of light
Envelopes wrap each little trace
Forks refresh their clock-anchor place
Tests hop cleanly through the night

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 8 files. (3 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: routing simulated device logs through HostLogger.
Description check ✅ Passed The description directly explains the logging integration, compatibility-level ordering, backend scope, test coverage, and ABI initialization changes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 22.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 8 files. (3 skipped: 3 unsupported.)


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ChaoWao

ChaoWao commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

I pushed one commit on top of yours (a02ac3f7a) rather than only commenting, since the fix is small and has a failing repro. Squash or drop it as you see fit — your 04a920a43 is untouched.

First, a correction to my review. I said this PR introduced the duplicated file sink. It does not. host_log.cpp is already compiled into _task_interface, libcpu_sim_context.so, the sim host/AICore SOs, and every generated orchestration SO (cmake/host_log_sources.cmake + kernel_compiler.py:290), and each copy has its own host_log_file_sink(). The orchestration SO logs and is dlopened, so the hazard predates this change. What your PR does is add one more instance on a high-traffic SO that DeviceRunner::unload_executor_binaries() dlcloses at every teardown.

The defect. HostLogFileSink held a raw FILE* with no destructor. dlclose unmaps the 1 MiB stdio buffer without flushing it, so a module's pending records die with its mapping. Only WARN and above write through, which makes DEBUG/INFO/TIMING — the bulk of sim AICPU traffic — exactly what is lost whenever a bound output_prefix sends records to the file.

Measured with a consumer .so emitting 200 INFO records, then unloaded, deterministic across repeats:

records reaching host.<pid>.log
before 159 / 200
after 200 / 200

The fix is a destructor on the sink (+13 lines). dlclose does run a function-local static's destructor, so the tail lands on disk. It closes only a stream this process opened — an inherited one holds records the parent still owns, and flushing that copied buffer would duplicate them, which is the invariant write_log_file already protects.

Also included:

  • test_host_log_dso_unload + a small consumer .so. Verified it fails at 159/200 with the fix reverted, per discipline.md §3. Named to avoid colliding with Fix: make host logging nonblocking and unify sim output #2029's test_host_log_cross_dso.
  • A missing case in test_sim_device_log: bind_level() clears log_directory_bound every time, so the sim file path had no coverage at all. Note this one passes with or without the fix — a single binary cannot observe a cross-DSO unload; it closes the destination gap, not the defect.
  • The six stale spots in docs/logging.md — most importantly :136-138, which still told readers that folding sim's logger in "is tracked separately by issue [Code Health] Log subsystem — one clock, one grammar, one gate #1792 item 5", i.e. described your change as outstanding work. Plus a paragraph in host-trace.md: it described the buffering as per process, and it is per module, which is what makes unload observable.

On CI. The ut failure on a02ac3f7a is not from either of our commits. It is tests/ut/cpp/a2a3/test_hbg_submit_poison.cpp failing to compile against OrchestratorState::fatal / SharedMemoryHeader::orch_error_code after the src/common/host_build_graph/ relocation — a file neither of us touches. I confirmed by building your commit alone merged with current main (clean) and by checking out pure upstream/main (bf68bb6cf), where test_hbg_graph_submit_failure fails identically with nothing from this PR present. I have re-run the failed jobs against current main.

One thing worth your call: the destructor fix is arguably not item-5 work — it repairs a pre-existing hazard that this PR makes materially worse. If you or a maintainer would rather it land against main independently and get its own CI signal, the commit is self-contained and cherry-picks cleanly.

indigo1973 and others added 2 commits August 31, 2026 01:05
- Bind each simulation AICPU DSO to the process-owned HostLogger
  state before applying the compatibility log level.
- Route simulation device records through HostLogger so they share the
  host envelope, live threshold, and configured destination.
- Remove the duplicate simulation formatter/writer and its 2048-byte
  PIPE_BUF constraint while leaving the onboard CANN backend unchanged.
- Add live-threshold, envelope, concurrent-thread, and forked-process
  coverage for issue hw-native-sys#1792 item 5.
- Fully initialize existing ABI v2 log-directory fields for strict
  simulation AICPU builds.
Every DSO that compiles host_log.cpp owns a private buffered stream on the
process's host.<pid>.log, and HostLogFileSink held that FILE* with no
destructor. A dlopened module's pending records were therefore discarded with
its mapping: dlclose unmaps the buffer without flushing it.

Folding the simulated device log into HostLogger makes the sim AICPU SO such a
module, and a high-volume one. DeviceRunner::unload_executor_binaries() dlcloses
it on every teardown, so its DEBUG/INFO/TIMING records — which ride the buffer,
since only WARN and above write through — were lost whenever a bound
output_prefix sent them to the file. The generated orchestration SO has the same
shape and was already exposed.

Give the sink a destructor. dlclose runs it, so the tail reaches disk. It closes
only a stream this process opened: an inherited stream holds records the parent
still owns, and flushing that copied buffer would duplicate them.

test_host_log_dso_unload pins the contract with a consumer .so that emits 200
INFO records and is unloaded; 159 of them reach the file without the destructor.
test_sim_device_log gains the missing case for the bound log directory, which
had no coverage on the sim path at all.

Docs: logging.md described sim as an independent stderr backend with its own
level flags and no host prefix, and pointed at hw-native-sys#1792 item 5 as outstanding
work. host-trace.md described the file buffering as per process; it is per
module, which is what makes unload observable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ChaoWao
ChaoWao merged commit 5009a52 into hw-native-sys:main Aug 31, 2026
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants